This probably tries to address the same issue as #69. Do you have a (small) test case that fails without this patch?

Compare https://travis-ci.org/boostorg/serialization/builds/305474813 and https://travis-ci.org/boostorg/serialization/builds/301650246:
On develop the test_dll_exported test consistently crashes on Linux. I could also reproduce that locally (gcc 6.4.1 on Fedora 25).

Yes, that looks convincing. 😃

This looks promising. It would explain why the problem occurs on some environments an not in others. One thing that has slowed us down is that we've been using test_dll_exported to test this because that is where the problem showed up. Looking through the test suite, I see there is no actual test for the singleton. Clearly this would be useful. So if anyone has or can make one, I'd be interested in seeing it and likely incorporating such a test in the set of tests. This would be particularly valuable as apparently some are using the singleton module outside the serialization library. And the singleton has special considerations regarding visibility, inclusion and the like.

I confirm that the patch in this pull request fixes the crash I describe in #69:

$ cat bug.cc 
#include <boost/archive/binary_iarchive.hpp>
#include <boost/serialization/vector.hpp>
#include <sstream>
#include <vector>

void f()
{
  std::stringstream iss;
  boost::archive::binary_iarchive ar(iss);
  std::vector<int> out;
  ar >> out;
}

int main(int argc, char* argv[])
{
  return 0;
}

Without the patch:

$ ./bug
Bus error (core dumped)

With the patch:

$ ./bug
$ valgrind ./bug
==34751== Memcheck, a memory error detector
==34751== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==34751== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==34751== Command: ./bug
==34751== 
==34751== 
==34751== HEAP SUMMARY:
==34751==     in use at exit: 1,544 bytes in 16 blocks
==34751==   total heap usage: 16 allocs, 0 frees, 1,544 bytes allocated
==34751== 
==34751== LEAK SUMMARY:
==34751==    definitely lost: 0 bytes in 0 blocks
==34751==    indirectly lost: 0 bytes in 0 blocks
==34751==      possibly lost: 0 bytes in 0 blocks
==34751==    still reachable: 1,544 bytes in 16 blocks
==34751==         suppressed: 0 bytes in 0 blocks
==34751== Rerun with --leak-check=full to see details of leaked memory
==34751== 
==34751== For counts of detected and suppressed errors, rerun with: -v
==34751== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
$

Cheerz!

It seems you also need to rebase.

Hmmm - I'm not convinced here.

One of the main problems that this singleton attempts to address is the the notorious "static variable initialization sequence" issue. The original scheme guaranteed that that items would be be initialized only when first used. This change would break that I believe.

The only reason we're using a raw pointer anyway is to work around an issue which presents itself only in some older compilers. Given the complexity of dealing with this, I'd really prefer that someone - maybe you - make a boost singleton library - maybe based on this so that the serialization library could just include it.

That seems like an undue burden on @masterleinad that might lead to the problem never getting fixed :-( I would hope that it's possible to commit a fix without creating a new boost library.

I understand being cautious around these issues. But it's not like everything is alright right now. The situation is already broken, and we know it used to work before.

Hmmm -I believe it was always broken before - it just showed up on one new test test_dll_exported and only on some combinations of compiler/library/OS. As far as I know, there are no current bugs in the serialization library.

A change like this has the potential to cause all sorts of havoc. Has anyone actually ran all the library tests with this change?

@robertramey

Hmmm -I believe it was always broken before

AFAIK it was working before, specifically 1.63 is ok, 1.65.0 is ok, but not 1.65.1, see this issue dealii/dealii#5262

Even though the issue is affecting me, I would also urge caution here. Furthermore, it isn't clear to me that the problem is fully understood. It would really help confidence if someone who understands it could provide a more detailed analysis. @robertramey BTW I am also curious as to the reasons for the current complex structure with a function-local static and a static data member. Is this the right forum to ask about that?

Hmmm -I believe it was always broken before

No, from the duplicate report at #69:

I confirmed that the bug was introduced in b0a794d. There is no crash with b0a794d~ and a crash with b0a794d (which is from Feb 2017).

I know the bug was detected in b0a794d But that does not convince me that that it was introduced in that version. In other words, I think the bug is somewhere in the the standard library/compiler implementation and that the change in b0a794d only reveals what was only there. Our "fix" is not really a fix but rather a work around some phenomenon that we don't understand.

juanchopanza - the "complex" structure is the triumph of implementation over design. That is, it wasn't actually "designed". I started with something simpler but had to make it a little more complex in order to make it work. A simpler design fails to address a number of issues:

a) sequence of nested static objects. static object a refers to static object b etc. This means that b has to be created before a. But this is under control of the compiler/library before "main" is called - or not.

b) Compilers will optimize code by elimination of code not explicitly referred to. The serialization library creates static objects to contain functions for de-serialization of exported derived pointers. These pointers are accessed through the vtable of the abstract base class so the compiler never sees any reference to them. So some method has to be sure that the compiler doesn't throw them away. In some cases this problem will manifest itself as a failure to link an in others as a runtime error. Note that behavior usually varies between debug and release modes, between compilers and compiler versions and whether the library is deployed as shared library/DLL or static library. Also different tests show different problems. It's not trivial to make this work across all these combinations. If fact the number of combinations is so large, that it's not really possible to test them all. And since we're relying on undefined behavior here, testing is actually the only way to make a guarantee that the library works.

c) The above is a result of the design decision to permit deep serialization to objects through pointers. This is a key feature of the Boost Serialization library. There are libraries which don't do this - e.g. Cereal which as a consequence are much, much simpler. In many cases, they might be a better choice. But they are not comprehensive. The blessing/curse of the Boost serialization library is that one can use it serialize virtually any C++ data structure without having to tamper with the structure design itself. That is, one can more or less completely de-couple the serialization from the rest of one's program. This is very convenient for the library user, but comes at a heavy cost of complexity for the library developer/maintainer

@robertramey: Pragmatically, do you have evidence that the current code is "more correct" than it was before? I do very much sympathize with your desire to come to a well-understood, well-designed code structure. But I'm not sure anyone has the gumption to actually make it happen, and it would be a shame if the perfect stood in the way of improvement.

We know that the code used to work and broke with a particular commit (works just before that patch, fails just after it). Furthermore, this commit can be reverted without too much trouble IIRC. I'm not the expert in this area, but the commit message that went along with it ("Trying to get minGW to function for serialization library") at least doesn't explain what was wrong before and why the change made is correct. Do you recall why this particular change was made?

The change was made to address failure of all tests with mingw. These tests are all passing now. I don't recall for a fact whether it was this change or some subsequent one which made the mingw tests pass again.

So, moving forward, where do we find someone with a MingW system who could test the proposed patch to see what happens?

LOL - actually, since I made this change I installed a mingw system just to be able to test this. I don't remember whether I tested this specific change on it. FYI I use a Mac with OSX. I bought Parallels, installed Windows, ubuntu linux and Mingw. It was basically the only way I could track down these kind of problems. Short version - works great for these issues. So I could investigate this some more. But it still takes a lot of time because it likely ends in a game of whack-a-mole. Fix it for mingw and it breaks mdvc, fix that and it breaks gcc, etc.

meanwhile, @res2k could you please rebase to fix the conflict?

@robertramey -- since you seem to be the only one with a system on which this can be tested, would you mind testing the patch there?

I don't know what else to suggest: I'm not a BOOST maintainer, so the decision is not mine, but if this was in the projects I work on, I'd choose reverting a small part of a patch that we know broke something, over waiting for the ultimate patch that is, in all likelihood never going to arrive :-(

Short of that, do you have any suggestions how we can proceed with this?

Just for clarification: The first commit of this PR was merged meanwhile (7d216b4) leaving the fix of the possible thread-safety issue in this PR.

Ah, thanks for the clarification and for rebasing what's left. The remainder appears to be a pretty straightforward change.

I too ran into a problem with 1.65.1 where my historic raw archives using boost serialization failed to import. I'm using msvc VS2017 15.3 and 15.4.2. Is there any easy way I can grab a whole file replacement to patch this problem? I would drop back to boost 1.64 but I've already updated my msvc compiler to 15.3 and 1.64 does not recognize the compiler. And to make things worse we use offline installers and no one has VS2017 15.2 anymore!

I also just tried boost 1.66 and my problem still remains.

I took another look at the issue.

For one, I tried to understand the reason behind the destruction order issue. As a local static variable is initialized lazily, or possibly not at all, it's cleanup function will be registered dynamically as well (See eg https://godbolt.org/g/YkApUv - note the _cxa_atexit call). That dynamic registration appears to make some mistake - the cleanup function is associated with a shared library that's cleaned up earlier than the module the static variable actually "lives" in is cleaned up - with code executed later assuming the variable is still valid.
That scenario can be worked around by using a global static var to handle the cleanup.

However, other ordering issues appeared with MSVC static builds. There the "solution" is to actually use atexit()…

So, on the one hand, that all appears to work (at least the automated tests look promising, see here: https://travis-ci.org/res2k/serialization/builds/338525872 and here: https://ci.appveyor.com/project/res2k/serialization/build/1.0.31-develop). On the other, distinguishing the cases makes the implementation a bit messy. I'll probably try if I can find a cleaner yet one-size-fits-all implementation.

You are still making the mistake I described in #104: You have ctor/dtors for singleton, expected them to do something but never instantiate it. Hence the destroyed flag is never set! This causes crashes for certain destruction orders (e.g. extended_type_info_typeid singleton destructed after ktmap singleton (here:

@robertramey

I know the bug was detected in b0a794d But that does not convince me that that it was introduced in that version. In other words, I think the bug is somewhere in the the standard library/compiler implementation and that the change in b0a794d only reveals what was only there. Our "fix" is not really a fix but rather a work around some phenomenon that we don't understand.

The bug was introduced in that revision. See #104 for proof.

The original scheme guaranteed that that items would be be initialized only when first used. This change would break that I believe.

This is (currently and for a while) not true. The scheme used makes sure all singletons are created BEFORE main is entered (so one can use lock although I question its use)

Actually, is the singleton<> ctor even called?
Anyway, I'd try dealing with get_is_destroyed() in cleanup_func() as well.

The singleton is never created, so no. That is my point

I did a quick test on the branch for this PR: I put assert(false) in singleton ctor resp. dtor, on gcc/Linux; running the unit tests, these asserts generally fired.
(Please take note this branch is not merged into mainline serialization, so issues there may not apply here, and vice versa.)

Then where is it constructed? I'll have another look too and contribute a test for the singleton itself

Ok found it. There is some trickery done there. Some classes are derived from singleton<T>. So singleton<T> will instantiate singleton_wrapper<T> which is derived from T and hence from singleton<T> (ouch, my brain!)

I think this is intended and should be the way singleton<T> has to be used as one can then simply use T::get_const_instance(). However for some classes this is not possible (hello std::map) which leads to problems as singleton<T>::get_const_instance() was not meant to be used. Maybe this should be enforced?

OK - I'm going to take another look at this. I wasn't happy with the original "fix", but things need to be fixed. So it looks like we'll have to fix this "fix". I hate this stuff. If some want's to submit a test which detects a memory leak, that might be useful to cut the "whack a mole" cycle and keep us moving forward.

Every type used in serialization registers itself with the serialization library. This is actually 2 singletons: One for the registered type and one for the register itself. During destruction of the registered type it unregisters itself.

The problem was, that the register was destructed before the registered type and so unregistering lead to a crash due to the destruction check not working. This was partially fixed by using a heap allocated singleton instance which "fixed" it by leaking the register singleton (and possibly some more)

And no the user can't do much about this. Use Boost >= 1.66 and don't use serialization in a shared library that is loaded/unloaded multiple times as this will build up wasted memory.

Imo it cannot be avoided. The singleton gets instantiated for every type used (I think)

No other solution possible than fixing it.

I'm working on it too (look at my fork. I got test cases atm and will fix them in the next days)

Ok I have my own branch ready. Waiting for the tests at https://ci.appveyor.com/project/Flamefire/serialization and https://travis-ci.org/Flamefire/serialization

The branch is https://github.com/Flamefire/serialization/tree/feature/singleton-fix

@alfC I added a PR which you can use to batch your version of boost to avoid the problem in the meantime. If you encounter any problems, tell me in that PR: #105

I believe this is now addressed. If anyone is unhappy with the resolution, open a new PR

@robertramey: For completeness, could you say which pull request/commit fixed the issue? This way it's easier for others later on to follow the conclusion of the discussion.

sorry I can't do this. the initial fix was just reversing the latest commit to fix another problem. Once the this was done, the original problem reared it's ugly head generating a months long saga which only recently has started getting sorted out. You can reconstruct all this yourself if it's important to you, but I personally can't spare the time to do this.

@bangerth The commit was f297d80 which is mostly my PR #131
This might not be 100% correct, but answers your question